Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typescript support for clickOutside directive #265

Closed
wants to merge 1 commit into from

Conversation

astagi
Copy link

@astagi astagi commented Oct 22, 2020

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.227% when pulling d1dcee8 on astagi:feature/fixtypescript into 82f131f on ndelvalle:master.

@astagi astagi mentioned this pull request Oct 22, 2020
@deefour
Copy link

deefour commented Oct 23, 2020

I don't think a shorthand ambient module is correct way for a library to implement Typescript support. I'd prefer to see TypeScript error out when importing this module, requiring I acknowledge it is missing proper type declarations by adding the ambient module to my own .d.ts file.

@ndelvalle
Copy link
Owner

@astagi this is a bit hacky, it make it looks like we have TS but we actually don't 😅

@astagi
Copy link
Author

astagi commented Oct 24, 2020

@deefour I know this, you're absolutely right! This is not the correct way to implement TS, this is just a hacky fix to support TS rapidly, avoiding errors when you import this module. @ndelvalle let me know if you want to merge this PR and release a new version with this fix or if you prefer waiting for a better TS support, in the latter case I would convert this PR to draft and start working on TS support

@ndelvalle
Copy link
Owner

@astagi I prefer to have a better TS support for sure 🙂

@astagi
Copy link
Author

astagi commented Oct 26, 2020

Ok @ndelvalle this PR switches now to WIP 😉

@astagi astagi marked this pull request as draft October 26, 2020 15:03
@ndelvalle ndelvalle closed this Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants